-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
update optional labels #174
update optional labels #174
Conversation
charts/agent-stack-k8s/values.yaml
Outdated
labels: | ||
user: "user" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that the default in the JSON schema is {}
, I think we should do that here as well.
labels: | |
user: "user" | |
labels: {} |
Also, this does not fit the JSON schema you've specified. Attempting to deploy it will give:
Error: UPGRADE FAILED: values don't meet the specifications of the schema(s) in the following chart(s):
agent-stack-k8s:
- labels.user: Invalid type. Expected: object, given: string
But as per my comment above, I don't think the JSON schema is valid for Kubernetes.
"additionalProperties": { | ||
"type": "object", | ||
"default": {} | ||
}, | ||
"example": [ | ||
{ | ||
"user": { | ||
"user": "name" | ||
}, | ||
"project": { | ||
"project": "project" | ||
} | ||
} | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these valid labels? My understanding is that labels are key-value pairs where both the key and value are strings. But here it seems you're asking for the values to be objects.
I tried to deploy this from your branch with
labels:
user:
user: "name"
project:
project: "project"
which is the example given and passes the JSON schema, but Kubernetes does not accept it with the error message:
json: cannot unmarshal object into Go struct field ObjectMeta.spec.template.metadata.labels of type string
I think it needs to be something like:
"additionalProperties": { | |
"type": "object", | |
"default": {} | |
}, | |
"example": [ | |
{ | |
"user": { | |
"user": "name" | |
}, | |
"project": { | |
"project": "project" | |
} | |
} | |
] | |
"additionalProperties": { | |
"type": "string", | |
"default": "" | |
}, | |
"example": [ | |
{ | |
"user": "name", | |
"project": "project" | |
} | |
] |
@@ -11,6 +11,7 @@ spec: | |||
metadata: | |||
labels: | |||
app: {{ .Release.Name }} | |||
{{- toYaml $.Values.labels | nindent 8 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If a user sets
labels:
app: foo
then the selector will no longer match the label. I think this is a footgun we should not allow.
I recommend you merge app: {{ .Release.Name }}
back into $.Values.labels
to prevent this, and document that the app
label will be overwritten if it is specified by the user.
To implement this, you most likely will need to create a _helpers.tpl
file and make some helpers in it.
Have a look at what the helm chart that is generated when you run
helm create agent-stack-k8s
does.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this @jiaquan1, we're getting there, but I have some more suggestions.
@@ -0,0 +1,6 @@ | |||
{{/* Generate basic labels */}} | |||
{{- define "mychart.labels" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{{- define "mychart.labels" }} | |
{{- define "agent-stack-k8s.labels" }} |
Let's give a more meaningful name to the helpers
labels: | ||
app: {{ .Release.Name }} | ||
{{- toYaml $.Values.labels | nindent 8 }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's still going to be the case that if the user specifies, e.g., --set labels.app=orcas
that the labels don't match the selectors. This is the error you get when you try to upgrade an existing release
Error: UPGRADE FAILED: cannot patch "agent-stack-k8s" with kind Deployment: Deployment.apps "agent-stack-k8s" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app":"orcas"}: `selector` does not match template `labels`
And this is what you get when you try to install a new release:
Release "agent-stack-k8s" does not exist. Installing it now.
Error: 1 error occurred:
* Deployment.apps "agent-stack-k8s" is invalid: spec.template.metadata.labels: Invalid value: map[string]string{"app":"orcas"}: `selector` does not match template `labels`
There are few ways to approach this.
You can simply reverse the order of the app: ...
and {{- toYaml ...
lines. This will give the later one preference. You don't need a separate _heplers.tpl
file to do this. But this approach also has a footgun as it silently overwrites the app
value that the user supplied. Furthermore, it leads to the output of helm template
containing duplicate keys, which is technically invalid yaml. Some users might process the output of helm template
with yaml parsers that would reject it for this reason, so I don't think this is acceptable for us in the long term.
A better approach is to use helper functions to detect when the user is specifying an app
label and return an error to the user letting them know that app
is reserved. That's where helper functions come in. You should be able to use something like this: https://masterminds.github.io/sprig/flow_control.html to achieve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, typically, you want there to be no indentation in the helper function so that all the indentation is managed in the template with nIndent
.
@@ -10,7 +10,7 @@ spec: | |||
template: | |||
metadata: | |||
labels: | |||
app: {{ .Release.Name }} | |||
{{- template "mychart.labels" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think include
is what's needed here
{{- template "mychart.labels" }} | |
{{- include "mychart.labels" }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working with us on the feedback @jiaquan1. This looks really close!
Co-authored-by: Narthana Epa <narthana.epa@gmail.com>
Co-authored-by: Narthana Epa <narthana.epa@gmail.com>
labels: | ||
{{- toYaml $.Values.labels }} | ||
app: {{ .Release.Name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
labels: | |
{{- toYaml $.Values.labels }} | |
app: {{ .Release.Name }} | |
{{- toYaml $.Values.labels }} | |
app: {{ .Release.Name }} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise, labels
appears twice and there will be another error:
Error: YAML parse error on agent-stack-k8s/templates/deployment.yaml.tpl: error converting YAML to JSON: yaml: line 14: mapping values are not allowed in this context
This is what the template renders as:
# Source: agent-stack-k8s/templates/deployment.yaml.tpl
apiVersion: apps/v1
kind: Deployment
metadata:
name: agent-stack-k8s-clusters
namespace: buildkite-clusters
spec:
selector:
matchLabels:
app: agent-stack-k8s-clusters
template:
metadata:
labels:
labels:user: user
app: agent-stack-k8s
if you run it with --set labels.user=user
. Notice that the indentation is also not consistent. I will repeat my recommendation that you DO NOT do any indentation in the helper, and use nindent
(not indent
) in the yaml file to control the indentation.
I recommend you attempt to render the template with help template --debug
during your development, so avoid having to go back and forth like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. I am not very familiar with helm cmd. help template --debug
command did not work in my git repo or I missed some pre-requisition step
help template --debug
bash: help: no help topics match `--debug'. Try `help help' or `man -k --debug' or `info --debug'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was a typo, its helm
, not help
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did it! 🎉
Update the PR after revert #173 (review)